Code Review — PR #907

Solid diagnostic improvement overall. The changes directly address the MAL-57 post-mortem gaps (stdout vs stderr, memory-safe tail read, ps latency bound). A few things worth flagging:

Correctness

logTail returns maxLines + 1 when truncated

The truncation marker is inserted after .suffix(maxLines), so logTail(path:, maxLines: 200) can return 201 lines when the tail window is truncated. Non-truncated calls return exactly maxLines. The tests capture this behavior correctly (testLogTailTruncatedReadDropsPartialFirstLine asserts 11, not 10), but callers may be surprised — and the capture file will have one extra line in the common-case where logs exceed 1 MB. Consider either:

• Capping at maxLines by taking suffix(maxLines - 1) before inserting the marker, or
• Documenting the off-by-one in the function signature comment so future callers know

Trailing empty line when log files end with \n

components(separatedBy: .newlines) on a \n-terminated file produces a trailing "". logTail doesn't strip it, so the last element of the returned array may be an empty string. Real kanata logs almost certainly end with \n. The test fixture uses joined(separator: "\n") without a trailing newline so the tests don't catch this. The original tailOfFile had the same issue — this PR carries it forward rather than introducing it — but it's worth fixing since we're touching the function anyway. A .filter { !$0.isEmpty } before .suffix(maxLines) would close it.

Style

await placement

await lines.append(contentsOf: topCPUProcessLines(limit: 5))

This compiles (the await covers the whole expression including the async argument), but the more idiomatic form makes clear which sub-expression is async:

lines.append(contentsOf: await topCPUProcessLines(limit: 5))

Minor, but the second form is what you'd expect to see in Swift concurrency code.

What's good

• Lossy UTF-8 decode (String(decoding:as:)) correctly handles the mid-multibyte-character seek boundary that would have broken the whole chunk with String(data:encoding:). The test with "héartbéat" padding verifies this.
• SubprocessRunner.shared + 5s timeout — exactly right. A hung ps under CPU starvation is precisely the condition being diagnosed; bounding it prevents the snapshot from stalling the recovery restart.
• Filter-then-suffix ordering — filtering spam before .suffix(maxLines) is correct. Post-filter, maxLines useful lines are what the analyst actually sees.
• Truncation marker — the "(… older output truncated…)" prefix is the right signal that filtered spam may have evicted earlier useful lines from the tail window.
• Test coverage — the four new tests cover the edge cases that matter: spam filtering with false-variant survival, missing file, UTF-8 boundary on truncated read, and subprocess skip in test environment. All well-structured.
• KeyPathConstants.Logs for log paths — no re-hardcoded strings.

Non-blocking

The two correctness items above are both defensive hardening on diagnostic capture code, not the hot path, so they don't block this. The trailing-empty-line issue in particular is pre-existing. Tagging as suggestions for the next pass if you agree with the direction.

Addressed the claude-review feedback in ba0bc38: logTail now drops empty lines (including the trailing-newline split artifact — fixture updated to use a \n-terminated file), and the truncation marker's +1 over maxLines is documented in the signature comment. Declined the await placement suggestion: the pinned SwiftFormat 0.61.1 config hoists await to statement position, so the suggested form gets rewritten back on every format.

Code Review — PR #907

Overview

Solid, well-scoped incident-capture improvement. The 2026-06-10 MAL-57 incidents being useless because (a) the wrong log file was captured and (b) the GUI was closed is exactly the kind of post-incident hardening that pays off. The implementation is clean and the test coverage is thorough.

What's working well

• Bounded 1MB tail over the previous full-file load is the right call for a 175MB log.
• Lossy UTF-8 decode (String(decoding:as:UTF8.self)) + dropFirst() on truncated reads correctly handles the seek-landing-mid-multibyte-character case. This is the right fix.
• SubprocessRunner.shared with timeout: 5 instead of raw Process() bounds the worst case under the very CPU starvation being diagnosed — a self-inflicted stall in the recovery path was a real risk.
• KeyPathConstants.Logs paths eliminate the re-hardcoded strings the first pass had.
• %.2f formatting for loadavg is a nice readability improvement over raw Double interpolation.
• Adding pid to -Aceo pcpu,pid,comm makes process snapshots correlatable with other diagnostics.
• 4 new unit tests covering spam filtering, partial-line drops, UTF-8 boundary, missing file, and test-environment ps skip — the truncated-read multibyte test in particular is exactly the kind of regression test that earns its weight.

Minor observations

1. await on a mutating append call (style)

In writeSnapshotToDisk and systemLoadLines:

await lines.append(contentsOf: systemLoadLines())
await lines.append(contentsOf: topCPUProcessLines(limit: 5))

This is valid Swift — await on an expression containing an async call works — but it's easy to misread as if append is async. The two-line form is unambiguously clearer:

let loadLines = await systemLoadLines()
lines.append(contentsOf: loadLines)

Low priority, but worth a nit if you're touching this file again.

2. Empty result when all lines are filtered (non-truncated read)

If the tail window fits under 1MB (so offset == 0) but every line in the file matches the spam filter, logTail returns [] with no marker. An incident file section that silently contains nothing is mildly confusing. The truncation marker path (offset > 0) correctly signals partial reads, but there's no equivalent signal for "filter removed everything." In practice this shouldn't happen (the stdout log always has non-heartbeat lines), so this is low priority — just worth being aware of.

3. topCPUProcessLines visibility

The old topCPUProcesses was private; the new topCPUProcessLines is internal (package-level) for testability. This is tested indirectly via systemLoadLines() in testSystemLoadLinesSkipProcessListingInTests, so the extra surface exposure isn't strictly needed. That said, direct testability of the test-environment guard is a reasonable trade-off. No action needed.

4. Truncation marker count guarantee

The docstring says the result "can be maxLines + 1 long." The test asserts tail.count == 11 (10 lines + marker). This is correct, and the testLogTailTruncatedReadDropsPartialFirstLine test validates it. Just confirming the off-by-one is intentional and documented — it is.

Summary

No blocking issues. The code is correct, the edge cases are handled, and the tests cover the important regressions. The await append(contentsOf: asyncFn()) style is the only thing worth a follow-up cleanup if convenient.

Verdict: Approve — ready to merge.

Code Review - PR 907

Overview

Enriches StuckKeyRecoveryService incident captures with: a filtered stdout tail (1MB cap, heartbeat spam stripped), a bounded system-load snapshot (loadavg + ps via SubprocessRunner with timeout), and a documented GUI-only limitation. The PR correctly identifies why the 2026-06-10 captures were useless and directly addresses it. Implementation is clean and well-tested.

Strengths

• Memory-safe tail read. logTail uses FileHandle.seek to read only the trailing 1MB. The previous tailOfFile had a 64KB default that was a latent footgun for 175MB+ stdout logs.
• Lossy UTF-8 decode. Using String(decoding:as:) instead of String(data:encoding:) is exactly right. A seek landing mid-multibyte character would silently fail the strict decode and return (file not readable) for the entire tail. The partial first line is now correctly dropped with dropFirst().
• Explicit truncation marker. The sentinel prepended at index 0 prevents a capture dominated by filtered spam from masquerading as complete history. Good discipline.
• SubprocessRunner + timeout. Replacing raw Process() with a 5s-timeout SubprocessRunner.shared is the right call. A starved ps under exactly the CPU pressure being diagnosed could otherwise stall the recovery restart indefinitely.
• pid added to ps output. -Aceo pcpu,pid,comm vs the previous -Aceo pcpu,comm is a low-cost improvement -- process name alone is ambiguous when multiple instances run.
• Test environment guard encapsulated. Moving TestEnvironment.isRunningTests inside topCPUProcessLines rather than at the call site is cleaner and harder to accidentally bypass.
• Test coverage. Four focused tests cover the core edge cases: spam filtering with false-variant survival, missing file, truncated-read partial-line/UTF-8, and test-environment skip. The truncation test is particularly thoughtful -- a greater-than 1MB fixture with multibyte characters that can straddle the seek boundary.

Issues

Minor: unusual async expression form

In writeSnapshotToDisk and systemLoadLines, both call sites use:

await lines.append(contentsOf: systemLoadLines())
await lines.append(contentsOf: topCPUProcessLines(limit: 5))

Valid Swift (the compiler propagates await into the async argument), but unusual enough to cause a double-take. The conventional form is clearer:

let loadLines = await systemLoadLines()
lines.append(contentsOf: loadLines)

Not a bug -- readability nit only.

Minor: truncation marker reports "1024KB" for the default 1MB window

With maxBytes = 1024 * 1024, the message reads "1024KB" rather than "1MB". Not wrong, but slightly confusing in incident files. Low priority.

Test nit: testLogTailFiltersSpamAndLimitsLines does not assert the truncation marker is absent

The fixture is ~20KB (under the 1MB cap), so no marker is prepended. But the test asserts tail.count == 200 without confirming the file was not truncated. If the fixture ever crosses 1MB, the count assertion breaks confusingly. Consider adding an assertion that tail.first does not contain "truncated".

Non-Issues

• logTail returning up to maxLines + 1: Documented in the function comment; the truncation test correctly asserts 11 lines for maxLines: 10. Callers have no hard budget.
• Case-sensitive substring filter: Fine for "virtual_hid_keyboard_ready true" -- kanata log format is stable.
• Capture-then-restart ordering unchanged: Confirmed by PR description and diff. New async work is bounded (two <=1MB reads + timeout-capped ps).

Verdict

Approve with minor nits. Core logic is correct, memory-safety improvement is important, and test coverage is solid. The two cosmetic issues ("1024KB" vs "1MB", async expression style) can be addressed here or in a follow-on.